-
Notifications
You must be signed in to change notification settings - Fork 828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NETBEANS-791] Highlights for the top line of JavaDoc are done on every line. #550
Conversation
@@ -55,21 +57,26 @@ | |||
public class Highlighting extends AbstractHighlightsContainer implements TokenHierarchyListener { | |||
|
|||
private static final Logger LOG = Logger.getLogger(Highlighting.class.getName()); | |||
|
|||
private static final String WS = " \t\n"; // NOI18N | |||
private static final String JAPANESE_PERIOD = "。"; // NOI18N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the character does not fall into ANSI/ISO-8859-1 charset, better encode it as unicode escape in the java source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "。"
, use "\u3002"
i.e. private static final String JAPANESE_PERIOD = "\u3002";
Is this OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
String period = null; | ||
int indexOfPeriod = -1; | ||
for (String p : PERIODS) { | ||
int index = TokenUtilities.indexOf(seq.token().text(), p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more appropriate if Javadoc lexer returned some specific tokenID japanese period char (and ordinary period char), since the period after the 1st sentence is a lexical element. But I fear that a new token ID could break existing lexer clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear that a new token ID could break existing lexer clients.
Me, too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdedic So, I would like to avoid changing the JavadocLexer(java.lexer) because a new tokenID(e.g. JavadocTokenId.JAPANESE_DOT
) is needed only for this feature at the moment. Even if someone adds the new tokenID, probably, we can notice that because I added unit tests to this feature.
However, if you prefer adding the tokenID, I'll try it :)
if (seq.token().id() == JavadocTokenId.DOT) { | ||
if (seq.moveNext()) { | ||
if (isWhiteSpace(seq.token())) { | ||
return new int [] { start, seq.offset()}; | ||
} | ||
seq.movePrevious(); | ||
} | ||
} else if (period != null && indexOfPeriod != -1) { | ||
// NETBEANS-791 | ||
int offset = TokenUtilities.indexOf(seq.token().text(), period) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't TokenUtilities.indexOf(seq.token().text(), period)
the same value as indexOfPeriod
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Thank you for catching it.
@sdedic Should I add the tokenID? or Can I merge this? |
I didn't study this in detail, but should JavadocTI.DOT be also used for
the Japanese dot? I.e. there would be a single token ID returned for both
(all) kinds of dots. In any case, I'd keep the test for highlighting, that
seems useful to ensure the functionality does not regress.
|
Possibly. But |
I'm not sure where impact area of that change is because I'm not an expert of Java area... |
My Proposal:
That way, someone other than me also may be able to work on it. I suppose that the person can do it even if the person doesn't know the Japanese language because I added test cases for it. |
@junichi11 OK, let's do gradual improvements. |
@sdedic Thank you! Will merge this. |
I added unit tests.
https://issues.apache.org/jira/browse/NETBEANS-791